Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #699 serdes missed on items in a collection. #704

Conversation

robertjustjones
Copy link
Contributor

@robertjustjones robertjustjones commented Feb 12, 2022

In the schema preprocessor recursion, items was left out of the handling for allOf, oneOf, anyOf, properties.

In my addition, I have commented out a check for schema.type == 'array'. Please advice on whether it's prudent to add or should be dropped out.

The test 699.spec.ts fails without this fix and passes with it. Alternatively, serdes.spec.ts could incorporate that change.

One note, I use Date rather than DateTime because json serialization is doing date.toString, so the serdes code isn't really being tested for date-time. Perhaps changing the test to use any other date format that toISOString would help.

#699

@robertjustjones robertjustjones force-pushed the issue-699-serdes-items-in-collection branch from c16ab96 to 8af4384 Compare February 12, 2022 19:06
@robertjustjones robertjustjones force-pushed the issue-699-serdes-items-in-collection branch from 8af4384 to 77dfd37 Compare April 5, 2022 19:44
@robertjustjones
Copy link
Contributor Author

Rebased in renewed hope of getting merged.

@Fabiencdp
Copy link

Coming here after a bunch of error like this :
.response.items[0].createdAt should be string
with this kind of declarations:

                  "items": {
                    "type": "array",
                    "items": {
                      "type": "object",
                      "required": ["createdAt"],
                      "properties": {
                        "createdAt": {
                          "type": "string",
                          "format": "date-time"
                        }
                      }
                    }
                  }

I was not getting this errors before, so that strange...

I tried your MR and i confirm your fix work well!
Adding schema.type == 'array' looks good to me, but you should use a strict equality operator for code consistence.

Hope it will be merge soon!

@@ -199,6 +199,9 @@ export class SchemaPreprocessor {
const child = new Node(node, s, [...node.path, 'anyOf', i + '']);
recurse(node, child, opts);
});
} else if (/*schema.type == 'array' && */ schema.items) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

schema.type === 'array' ?

Copy link
Owner

@cdimascio cdimascio May 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes schema.type array is correct here. Let’s get that updated and I’ll merge this in

Copy link

@Fabiencdp Fabiencdp Jun 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertjustjones Can you add it to your merge request ? or i'll do it later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'll get that done this morning. Sorry that I didn't see the comment earlier.

@robertjustjones robertjustjones force-pushed the issue-699-serdes-items-in-collection branch from 77dfd37 to 10519ec Compare June 16, 2022 15:28
@robertjustjones
Copy link
Contributor Author

@Fabiencdp this was more work that I anticipated because the serdes test has been changed. What's odd is that when I run 699.spec with the "array" lines commented out, I would expect the test to fail but it's passing. I think something solved the array issue more subtly, perhaps bumping AJV. Could that be? There's no sense adding the code (202-204 in schema.preprocessor) if it's not needed. I would feel more comfortable getting tests for "array" in serdes.spec, but without them failing it's hard to see if I have them correct.

I've left my commit unsquashed it you'd like to look, not intending for them to be merged as is. Let me know about the above and I can resubmit.

Also, very happy to see the dicer issue addressed.

@Fabiencdp
Copy link

@robertjustjones, i'll check as soon as i can, I had some good test cases on a living application

@Fabiencdp
Copy link

I just tried with the last master version (4.14.0-beta.2), builded locally, imported to the live project, still have the serdes errors about dates.
Then i added your fix (202-204 in schema.preprocessor), build it again, and it work well!

I didn't worked on the test for the moment, i'll try in the weekend.

@Fabiencdp
Copy link

Fabiencdp commented Jun 24, 2022

Hi @robertjustjones, can you tell me wich test you expect to fail ?

EDIT: get it, should be the "should control GOOD id format and get a response in expected format" wich check for date format as string, so when commenting the 202-204, it should fail, am i right ?
Good catch

@Fabiencdp
Copy link

I did some test using a reduced an anonymized part of our project schema, get it to work correctly.
here is a gist:
robertjustjones@9631dfd#diff-79c6ff5eb8a158b26e0c693cf08386a520d8bd27866f170ca808134d17317d5fR212 (it's an ugly WIP, please don't pay attention to linter stuff)

@Fabiencdp
Copy link

Fabiencdp commented Aug 23, 2022

@robertjustjones,
I digged about the test who should not pass without your fix, I found that it pass only when the array use a $ref.
Instead, if you define the schema directly, the test fail as expected.
This may be the reason why this bug has not yet been discovered.

To be clear, without the fix, the initial problem is:
Date object is not converted to a date-time string, and validating against a schema property defined as date-time fail with this kind of error: "path/0/prop must be string"

For example, let say this part of schema:

       history:
          type: array
          items:
            type: object
            properties:
              updateDate:
                type: string
                format: date-time

with this response object :

let json = {
  history: [
    {
      updateDate: new Date()
    },
  ],
};

Will return {message: "/response/history/0/updateDate must be string", code: 500} wich is not good. Date is a valid date.

But, if you define the schema with a $ref, like this:

        history:
          type: array
          items:
            $ref: "#/components/schemas/HistoryItem"

The validator don't throw and response is valid: { history: [ { updateDate: '2022-08-23T21:46:36.588Z' } ] }
It look's like serdes serialization is only used with $ref

So... What should we do ?

Did we must find why it happen ? It could be a problem with the way that reference are parsed ?

Or did we should accept your fix ?

I'll dig a little more while waiting your opinion.

EDIT:
I think your fix is right, without it array items don't pass throught schemaVisitor method, so serdes are not applied to items.

@robertjustjones
Copy link
Contributor Author

@Fabiencdp does the latest commit with the modified 699.spec cover the case you mention with historyWithoutRef?

@Fabiencdp
Copy link

Nop, i didn't push my last test at the moment

@Fabiencdp
Copy link

Sorry, I misunderstood your last comment, I'll check that today

@Fabiencdp
Copy link

@robertjustjones, no, it does not cover the case, the schema must be :

        historyWithoutRef:
          type: array
          items:
            type: object
            properties:
              modificationDate:
                type: string
                format: date

Then, comment your fix (schema.preprocessor.ts l202-204)

The run the test, you should get a 500 with body {message: "/response/historyWithoutRef/0/modificationDate must be string", code: 500}

If you re-active your fix, the test pass with success.

@robertjustjones robertjustjones force-pushed the issue-699-serdes-items-in-collection branch from 4425793 to e2480e1 Compare August 25, 2022 13:50
@robertjustjones robertjustjones force-pushed the issue-699-serdes-items-in-collection branch from e2480e1 to fdebec6 Compare August 25, 2022 13:55
@robertjustjones
Copy link
Contributor Author

@Fabiencdp you were exactly right! The tests looks to be passing and failing (without the code fix) as expected now.

I squashed the commits. Let's gets this one merged. Thanks, so much!

Copy link

@Fabiencdp Fabiencdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work!

@Fabiencdp
Copy link

@cdimascio we also need your review

@srgg
Copy link

srgg commented Aug 31, 2022

@cdimascio when do you think you will have time to take a look at this PR?

@srgg
Copy link

srgg commented Sep 1, 2022

@robertjustjones I've faced with another allOf corner case and curious: if your changes also fixing that:

    post:
      requestBody:
        required: true
        content:
          application/json:
            schema:
              additionalProperties: false
              oneOf:
                - type: array
                  items:
                    $ref: '#/components/schemas/Variable'
                - $ref: '#/components/schemas/Variable'

  schemas:
    Variable:
      description: ""
      type: object
      required:
        - name
        - value
      additionalProperties: false
      properties:
        name:
          type: string
        value:
          type: string
          nullable: true

Check1, body:

{
  "name": "new variable",
  "value": "just new"
}

Fails and this is not expected:

{
  "code": 400,
  "message": "request/body must NOT have additional properties, request/body must NOT have additional properties",
  "errors": [
    {
      "path": "/body/name",
      "message": "must NOT have additional properties",
      "errorCode": "additionalProperties.openapi.validation"
    },
    {
      "path": "/body/value",
      "message": "must NOT have additional properties",
      "errorCode": "additionalProperties.openapi.validation"
    }
  ]
}

Check2, body:

[
  {
    "name": "new variable",
    "value": "just new"
  },
  {
    "name": "new variable",
    "value": "just new"
  }
]

Pass request validation as expected

@Fabiencdp
Copy link

@srgg

I'm not sure if "additionalProperties" is valid before "oneOf",
"additionalProperties" should be inside a schema of type object, so only in components/schemas/Variable

You should try to change this part:

    post:
      requestBody:
        required: true
        content:
          application/json:
            schema:
              oneOf:
                - type: array
                  items:
                    $ref: '#/components/schemas/Variable'
                - $ref: '#/components/schemas/Variable'

@srgg
Copy link

srgg commented Sep 1, 2022

@Fabiencdp It seems that you are tight, thank you. So it is another issue, I have turned schema validation and did not get any error about that invalid additionalProperties declaration.

UPD: One more proof that case I mistakenly broad up is not relevant for express-openapi-validator, it seems that allOf with additionalProperties a major issue at JSON schema level: https://stackoverflow.com/questions/22689900/json-schema-allof-with-additionalproperties

@robertjustjones
Copy link
Contributor Author

@Fabiencdp @cdimascio looks like this PR would also close out #666. Identical code fix with similar test.

@Fabiencdp
Copy link

@cdimascio
Allow me to revive you about this MR, if we can close this quickly :)
Thanks you

@sgoodluck
Copy link

@robertjustjones, I digged about the test who should not pass without your fix, I found that it pass only when the array use a $ref. Instead, if you define the schema directly, the test fail as expected. This may be the reason why this bug has not yet been discovered.

To be clear, without the fix, the initial problem is: Date object is not converted to a date-time string, and validating against a schema property defined as date-time fail with this kind of error: "path/0/prop must be string"

For example, let say this part of schema:

       history:
          type: array
          items:
            type: object
            properties:
              updateDate:
                type: string
                format: date-time

with this response object :

let json = {
  history: [
    {
      updateDate: new Date()
    },
  ],
};

Will return {message: "/response/history/0/updateDate must be string", code: 500} wich is not good. Date is a valid date.

But, if you define the schema with a $ref, like this:

        history:
          type: array
          items:
            $ref: "#/components/schemas/HistoryItem"

The validator don't throw and response is valid: { history: [ { updateDate: '2022-08-23T21:46:36.588Z' } ] } It look's like serdes serialization is only used with $ref

So... What should we do ?

Did we must find why it happen ? It could be a problem with the way that reference are parsed ?

Or did we should accept your fix ?

I'll dig a little more while waiting your opinion.

EDIT: I think your fix is right, without it array items don't pass throught schemaVisitor method, so serdes are not applied to items.

Just spent several hours on this and found your comment which solved. Looking forward to having this one merged! Thank you for finding.

@robertjustjones
Copy link
Contributor Author

@cdimascio many of us have spent a fair amount of time chasing this solved issue after the clear fix was known. Please do stamp this one approved and release it.

@robertjustjones
Copy link
Contributor Author

Hey @cdimascio would some coffee help get this merged?

@Fabiencdp
Copy link

The owner has no github activity since july 2022... hope he is well. A backup reviewer would help in this case...
I can try to send him an email, or should we fork this and declare this repository as dead?

@srgg
Copy link

srgg commented Nov 9, 2022

@Fabiencdp You can check the owner's activity for the previous year, it seems to me that the owner appears once in a while (in 6-7 months). Not a good schedule as for production usage, but I have no chance to support this repo :(. Therefore, the question is there a chance that somebody can fork it and re-bake community around a new repo

@robertjustjones
Copy link
Contributor Author

He's still recently active on LinkedIn, presumably just focused in other areas like we are. Adding reviewers and/or co-owners would seem to be the way to go to share the load.

@cdimascio cdimascio merged commit 77bc4ae into cdimascio:master Nov 19, 2022
@robertjustjones
Copy link
Contributor Author

Thanks @cdimascio !

@robertjustjones
Copy link
Contributor Author

Screen Shot 2022-11-22 at 2 24 13 PM

@cdimascio
Copy link
Owner

:) thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants